[refactor](be) Simplify exchange sink block sending#63574
Conversation
### What problem does this PR solve?
Issue Number: None
Related PR: None
Problem Summary: ExchangeSinkBuffer still carried a legacy _send_multi_blocks flag and mixed single-block and multi-block request assembly. This made the sending logic harder to reason about and left a compatibility branch in the sender. The change unifies request assembly around the blocks repeated field and uses exchange_multi_blocks_byte_size > 0 as the only switch; when the option is unset or non-positive, each RPC carries a single block.
### Release note
None
### Check List (For Author)
- Test: No need to test (logic-only refactor; verified diff and request flow)
- No need to test
- Behavior changed: Yes (unset/non-positive exchange_multi_blocks_byte_size now always uses single-block sending)
- Does this need documentation: No
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
run buildall |
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
/review |
TPC-H: Total hot run time: 31854 ms |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
I found one blocking compatibility issue. The refactor makes new exchange senders always serialize data in the repeated blocks field, even when exchange_multi_blocks_byte_size is unset or non-positive, and removes the legacy block sender path. Current receiver code still has request->has_block() explicitly marked as the old compatibility path, so during a rolling upgrade a new BE sender can send blocks to an old BE receiver that only understands block; that receiver will ignore the payload and only process EOS, causing dropped exchange data.
Critical checkpoints: goal/refactor is clear, but not safely accomplished because the protocol fallback is removed; change is small and focused; concurrency/lifecycle are not materially changed; no new config is added, but existing config semantics now no longer choose the legacy wire field; FE/BE protocol compatibility is the blocking concern; no transaction/persistence/data-write path is involved; no new observability appears necessary for this refactor; tests are missing for mixed-version or fallback behavior; user focus: no additional focus points were provided.
| for (auto& req : requests) { | ||
| if (req.block && !req.block->column_metas().empty()) { | ||
| auto add_block = brpc_request->add_blocks(); | ||
| add_block->Swap(req.block.get()); |
There was a problem hiding this comment.
This drops the legacy block wire path and always sends exchange payloads through blocks, even when _send_multi_blocks_byte_size <= 0 and only one block is sent. That breaks mixed-version BE compatibility: VDataStreamMgr::transmit_data() still treats request->has_block() as the old compatibility path, so an older receiver that does not handle field 13 will ignore this payload and only see EOS, losing exchange rows during rolling upgrade. Please keep using set_allocated_block() when multi-block exchange is disabled, or gate the repeated blocks field on a version/compatibility check.
TPC-DS: Total hot run time: 172687 ms |
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
2 participants
What problem does this PR solve?
Issue Number: None
Related PR: None
Problem Summary: ExchangeSinkBuffer still carried a legacy _send_multi_blocks flag and mixed single-block and multi-block request assembly. This made the sending logic harder to reason about and left a compatibility branch in the sender. The change unifies request assembly around the blocks repeated field and uses exchange_multi_blocks_byte_size > 0 as the only switch; when the option is unset or non-positive, each RPC carries a single block.
Release note
None
Check List (For Author)
Test: No need to test (logic-only refactor; verified diff and request flow)
Behavior changed: Yes (unset/non-positive exchange_multi_blocks_byte_size now always uses single-block sending)
Does this need documentation: No
What problem does this PR solve?
Issue Number: close #xxx
Related PR: #xxx
Problem Summary:
Release note
None
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)